Skip to content

security(audit): expire-stream griefing via paused top-up + deactivated DAO state gaps [F-1, F-2, F-3]#7

Open
Majormaxx wants to merge 1 commit into
jayteemoney:mainfrom
Majormaxx:security/audit-majormaxx-three-findings
Open

security(audit): expire-stream griefing via paused top-up + deactivated DAO state gaps [F-1, F-2, F-3]#7
Majormaxx wants to merge 1 commit into
jayteemoney:mainfrom
Majormaxx:security/audit-majormaxx-three-findings

Conversation

@Majormaxx
Copy link
Copy Markdown

Independent Security Audit — v1.0.0-rc1

Reviewer: Majormaxx
Date: May 18, 2026
Scope: stream-manager.clar, stream-factory.clar
Method: Full independent review of both contracts. All prior audit PRs (#2#6) and SECURITY_REVIEW.md reviewed before beginning to avoid duplication. Focused on fix-interaction effects, factory state-consistency, and the expire-stream recovery path.
Test run: npm test — 119/119 passing (113 original + 6 new)


Findings

F-1 — top-up-stream on paused stream enables permanent expire-stream griefing

Severity: Low-Medium | Function: top-up-stream | Lines: 686–693 (stream-manager.clar)

Root cause. The M-1 fix (dannyy2000) added expire-stream as a permissionless recovery path for paused streams whose end-block has passed. The L-10 fix (Zachyo) blocked top-ups on streams where stacks-block-height >= end-block. Neither fix covered the case where both conditions are approaching simultaneously: a sender who has paused a live stream can call top-up-stream repeatedly just before end-block arrives, each call extending end-block by ⌊amount × PRECISION / rate⌋ blocks.

expire-stream requires:

(asserts! (>= stacks-block-height end-block) ERR-STREAM-NOT-EXPIRED)

With end-block perpetually ahead, this condition never satisfies.

Cost to attacker. The L-8 zero-extension guard enforces a minimum top-up of ⌈rate / PRECISION⌉ tokens per call. However, every topped-up token enters the sender's own escrow and is fully recoverable via cancel-stream. The net out-of-pocket cost to indefinitely block expire-stream is zero.

Recipient impact. The recipient retains full access to earned tokens via claim (frozen at paused-at-block). What they lose is the ability to force a terminal state and recover the sender's unearned refund portion via the permissionless expire-stream path.

Fix. One assert added in top-up-stream after the CANCELLED/DEPLETED state guards:

;; Cannot top up a paused stream: extends end-block while earnings are frozen,
;; enabling the sender to delay expire-stream indefinitely with small repeated top-ups.
;; Resume the stream first, then top up.
(asserts! (not (is-eq status STATUS-PAUSED)) ERR-STREAM-PAUSED)

This guard is ordered before the end-block check (ERR-STREAM-ENDED), so both live-paused and expired-paused streams return ERR-STREAM-PAUSED (u203). The pre-existing L-10 regression test expected u207 for the expired-paused case and has been updated to u203 with an explanatory comment. Senders who legitimately want to extend a stream while it is paused must call resume-stream first — this is a reasonable constraint, as topping up a frozen stream is semantically inconsistent.


F-2 — Deactivated DAO can rename itself, freeing its locked name for third-party registration

Severity: Low | Function: update-dao-name | Lines: 99–124 (stream-factory.clar)

Root cause. register-dao writes to two maps atomically: daos (principal → record) and dao-names (name string → principal). deactivate-dao sets is-active: false on the daos record but does not touch dao-names. The intent is a soft-delete: the name stays reserved, the slot stays consumed.

update-dao-name reads the daos record via (unwrap! (map-get? daos caller) ERR-DAO-NOT-FOUND) but does not assert is-active. A deactivated DAO can therefore call it, triggering:

(map-delete dao-names old-name)   ;; frees "Acme DAO" in the reverse lookup
(map-set dao-names new-name caller) ;; claims "Acme DAO Reborn"

The map-delete makes the original name available. Any other principal can then call register-dao("Acme DAO") and take it.

Reproduction path:

wallet1 registers "Acme DAO"          → daos[wallet1].is-active = true, dao-names["Acme DAO"] = wallet1
wallet1 calls deactivate-dao          → daos[wallet1].is-active = false
wallet1 calls update-dao-name("Foo")  → dao-names["Acme DAO"] deleted
wallet3 calls register-dao("Acme DAO")→ succeeds — wallet3 now controls "Acme DAO" identity

Fix. New constant ERR-DAO-INACTIVE (err u507) plus an is-active guard at the top of update-dao-name:

(define-constant ERR-DAO-INACTIVE (err u507))
...
(asserts! (get is-active dao-data) ERR-DAO-INACTIVE)

F-3 — Deactivated DAO can call track-stream and inflate on-chain analytics

Severity: Low | Function: track-stream | Lines: 152–183 (stream-factory.clar)

Root cause. Structurally identical to F-2. track-stream reads (unwrap! (map-get? daos caller) ERR-DAO-NOT-FOUND) without checking is-active. A deactivated DAO can call track-stream to increment total-streams-created and total-deposited on its own record.

is-registered-dao returns false for deactivated DAOs, so external callers correctly see the DAO as inactive. However, the underlying analytics fields remain writable, creating a discrepancy: the DAO appears inactive to observers yet can inflate its on-chain track record — relevant for any governance system, grant program, or reputation layer that reads raw factory data.

Fix. Same ERR-DAO-INACTIVE guard applied to track-stream:

(asserts! (get is-active dao-data) ERR-DAO-INACTIVE)

Changes

File Change
contracts/stream-manager.clar +7 lines: STATUS-PAUSED guard in top-up-stream
contracts/stream-factory.clar +5 lines: ERR-DAO-INACTIVE (u507) constant + is-active guards in update-dao-name and track-stream
tests/stream-manager.test.ts +3 new tests, 1 updated (expected error u207 → u203)
tests/stream-factory.test.ts +4 new tests across update-dao-name and track-stream describe blocks
auditors-report/security-audit-report-by-majormaxx.md Full audit report with reproduction steps, impact analysis, fix rationale

Test Coverage

stream-manager.test.ts — top-up-stream describe block:

  • should reject top-up on a paused stream with ERR-STREAM-PAUSED — direct rejection
  • should allow top-up after resuming a paused stream — valid resume-then-top-up path preserved
  • should not allow top-up to bypass expire-stream on a paused stream near end-block — end-to-end: create stream → mine 50 blocks → pause → assert top-up rejected → mine past end-block → assert expire-stream succeeds

stream-factory.test.ts — update-dao-name and track-stream describe blocks:

  • should reject update-dao-name for deactivated DAO with ERR-DAO-INACTIVE
  • should not free a name when deactivated DAO rename is blocked — asserts third party cannot register the original name
  • should reject track-stream for deactivated DAO with ERR-DAO-INACTIVE
  • should not update DAO stats when track-stream is blocked after deactivation — asserts total-streams-created and total-deposited remain at zero

Confirmations

The following prior findings were independently verified as correctly implemented in v1.0.0-rc1:

Finding Author Verification
M-1: expire-stream settles paused streams dannyy2000 Token conservation holds across all test paths
M-2: two-step ownership transfer Zachyo / Ryjen1 propose + accept pattern correctly implemented; pending-owner cleared on accept
L-4: resume past end-block blocked Marvy247 ERR-STREAM-ENDED correctly returned
L-7: zero rate-per-block guard Sobilo34 deposit * PRECISION >= duration correctly enforced
L-8: zero-extension top-up guard Godbrand0 amount * PRECISION >= rate correctly enforced
L-9: pre-start pause blocked dannyy2000 current-block >= start-block correctly asserted
L-10: expired stream top-up blocked Zachyo stacks-block-height < end-block correctly asserted (now preceded by F-1 guard)
L-13: one-step ownership risk Ryjen1 Two-step pattern eliminates typo-based lockout

Verdict

Three findings — one Low-Medium, two Low. All fixed. No critical or high vulnerabilities found. Core streaming math, token conservation invariants, and the contract-caller authorization model are sound. F-1 closes the last known interaction gap between the M-1 and L-10 fixes. F-2 and F-3 complete the deactivate-dao state-lock that the factory's soft-delete pattern implied but did not enforce.

The contracts are ready for mainnet with these fixes applied.

… gaps

F-1: block top-up-stream on STATUS-PAUSED streams. A sender could repeatedly
extend end-block with minimum-cost top-ups, permanently preventing expire-stream
from triggering. Guard ordered before the end-block check; returns ERR-STREAM-PAUSED.

F-2: add is-active guard to update-dao-name. Deactivated DAOs could rename
themselves, freeing their locked name for third-party registration via map-delete.

F-3: add is-active guard to track-stream. Deactivated DAOs could increment
their own analytics post-deactivation.

New constant ERR-DAO-INACTIVE (u507) introduced for F-2 and F-3.
119/119 tests pass (+6 new, 1 updated).
@vercel
Copy link
Copy Markdown

vercel Bot commented May 18, 2026

@Majormaxx is attempting to deploy a commit to the dev_jaytee's projects Team on Vercel.

A member of the Team first needs to authorize it.

jayteemoney added a commit that referenced this pull request May 19, 2026
H-1 (Akanimoh, PR #5): stream-factory had no reactivate-dao - deactivated
DAOs were permanently locked out of the registry and their name burned in
dao-names forever. Added reactivate-dao public function with
ERR-DAO-ALREADY-ACTIVE (err u508).

F-1 (Majormaxx, PR #7): top-up-stream on a paused stream allowed sender to
extend end-block indefinitely with minimum-valid top-ups, permanently
griefing expire-stream. Added paused-state guard before the end-block check.

F-2 (Majormaxx, PR #7): update-dao-name had no is-active check, letting a
deactivated DAO release its locked name via map-delete and enabling name
squatting. Added ERR-DAO-INACTIVE (err u507) and is-active guard.

F-3 (Majormaxx, PR #7): track-stream had no is-active check, letting a
deactivated DAO inflate its own analytics post-deactivation. Same
ERR-DAO-INACTIVE guard.

Error-code numbering: ERR-DAO-INACTIVE=u507 (Majormaxx, 2 call sites),
ERR-DAO-ALREADY-ACTIVE=u508 (renumbered from u507 to avoid collision).

Test suite: 113 -> 125 passing tests (+12 covering all four fixes plus
updated L-10 test for new error ordering).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant